chore(core): tenant_organization_id for OS must be optional to allow clean ownership attribution to tenant or provider by pbreton · Pull Request #3689 · NVIDIA/infra-controller

pbreton · 2026-07-17T19:57:15Z

Initial implementation of templated OS assumed that any core-created OS (using gRPC API) not existing on the REST layer would be considered as 'owned' by provider (vs tenant). tenant_organization_id is effectively ignored during reconciliation when being OS is added to REST db.
This provides a cleaner model allowing for provider and tenant owned templated OS when created on the core side (instead of provider-only):

if tenant_organization_id is present then it is considered tenant owned during reconciliation
if tenant_organization_id is absent then it is provider owned during reconciliation

Type of Change

Change - Changes in existing functionality

Breaking Changes

This PR contains breaking changes

Testing

Unit tests added/updated
Integration tests added/updated
Manual testing performed
No testing required (docs, internal refactor, etc.)

Additional Notes

This is just the core/Rust side.

copy-pr-bot · 2026-07-17T19:57:25Z

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

coderabbitai · 2026-07-17T19:57:36Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 2420d119-b464-4b4e-b534-a70df75178e5

📥 Commits

Reviewing files that changed from the base of the PR and between 0e3dfd4 and 201366e.

📒 Files selected for processing (1)

crates/api-db/migrations/20260717120002_operating_systems_org_nullable.sql

Summary by CodeRabbit

New Features
- Operating system definitions can now be created without an owning organization.
- Organization information is preserved as optional across API requests, responses, CLI commands, and storage.
- CLI creation no longer requires the organization option.
Bug Fixes
- Empty organization values are rejected with a clear validation error.
- CLI and web displays safely handle operating systems without an organization.

Walkthrough

Operating system tenant organization identifiers are now optional across protobuf contracts, database persistence, API validation, CLI arguments, web display conversions, iPXE rendering, and related tests. Missing values persist as NULL and provided empty strings are rejected.

Changes

Nullable operating system organization

Layer / File(s)	Summary
Optional organization contracts and storage `rest-api/proto/...`, `crates/rpc/proto/forge.proto`, `crates/api-model/...`, `crates/api-db/...`, `crates/api-db/migrations/...`	Organization identifiers become optional in wire contracts, application models, database records, and the `operating_systems.org` column.
Nullable organization persistence `crates/api-db/src/operating_system.rs`, `crates/api-db/src/instance.rs`	Database creation and retrieval preserve both absent and provided organization values, with database fixtures updated for optional inputs.
API creation and rendering flow `crates/api-core/src/handlers/...`, `crates/api-core/src/ipxe.rs`, `crates/api-core/src/tests/...`, `crates/api-core/tests/integration/...`	Creation accepts omitted organization identifiers, rejects present empty strings, passes optional values to iPXE rendering, and updates integration coverage.
CLI and web optional-value adapters `crates/admin-cli/src/operating_system/...`, `crates/api-web/src/operating_system.rs`	CLI creation accepts omitted `--org` values, while CLI and web display mappings render missing organizations as empty strings.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant AdminCLI
  participant API
  participant Database
  participant IPXE
  AdminCLI->>API: Submit optional tenant_organization_id
  API->>API: Reject present empty string
  API->>Database: Persist organization or NULL
  Database-->>API: Return optional organization
  API->>IPXE: Pass optional tenant_id
  IPXE-->>AdminCLI: Render operating system data

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: making OS tenant_organization_id optional for tenant or provider ownership.
Description check	✅ Passed	The description matches the changeset and explains the new tenant-vs-provider ownership behavior clearly.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

pbreton · 2026-07-17T19:58:05Z

@coderabbitai full review

coderabbitai · 2026-07-17T19:58:11Z

✅ Action performed

Full review finished.

github-actions · 2026-07-17T20:03:36Z

🔐 TruffleHog Secret Scan

✅ No secrets or credentials found!

Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉

🔗 View scan details

_{🕐 Last updated: 2026-07-17 20:03:35 UTC | Commit: eb18a6a}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (2)

crates/api-core/src/handlers/operating_system.rs (1)
273-280: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the shared NicoError path for this validation.

This new user-input validation branch returns tonic::Status directly. Core handler guidance calls for NicoError::InvalidArgument converted with .into(), keeping error mapping consistent across handlers.

As per path instructions, handler validation should use uniform NicoError errors and .into() rather than raw Status.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/api-core/src/handlers/operating_system.rs` around lines 273 - 280,
Update the tenant_organization_id validation branch in the relevant
operating-system handler to return NicoError::InvalidArgument and convert it
with .into(), replacing the direct Status::invalid_argument construction while
preserving the existing validation message.
Source: Path instructions
crates/admin-cli/src/operating_system/create/args.rs (1)
39-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add worked after_long_help examples for the optional --org contract.

This change introduces two meaningful modes: omit --org for provider-owned definitions or provide it for tenant-owned definitions. Add the required EXAMPLES: help section with realistic invocations for both modes.

As per path instructions, admin CLI argument structs must include realistic worked examples for meaningful command modes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/admin-cli/src/operating_system/create/args.rs` around lines 39 - 44,
Add an after_long_help section to the org argument definition with an EXAMPLES:
heading and realistic worked command invocations showing both modes: omitting
--org for provider-owned OS definitions and supplying --org for tenant-owned
definitions. Keep the existing optional org behavior and help text unchanged.
Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/api-db/src/operating_system.rs`:
- Around line 421-443: Combine test_create_persists_null_org and
test_create_persists_org into one table-driven persistence test covering both
None and Some("acme") organization cases. Define explicit input and expected
organization values, then iterate each case through the existing create/get flow
while preserving assertions for both created.org and fetched.org.

---

Nitpick comments:
In `@crates/admin-cli/src/operating_system/create/args.rs`:
- Around line 39-44: Add an after_long_help section to the org argument
definition with an EXAMPLES: heading and realistic worked command invocations
showing both modes: omitting --org for provider-owned OS definitions and
supplying --org for tenant-owned definitions. Keep the existing optional org
behavior and help text unchanged.

In `@crates/api-core/src/handlers/operating_system.rs`:
- Around line 273-280: Update the tenant_organization_id validation branch in
the relevant operating-system handler to return NicoError::InvalidArgument and
convert it with .into(), replacing the direct Status::invalid_argument
construction while preserving the existing validation message.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c96e07dd-8b3c-4706-b5dc-fb6601ab2e05

📥 Commits

Reviewing files that changed from the base of the PR and between 0dae3ca and eb18a6a.

⛔ Files ignored due to path filters (1)

rest-api/proto/core/gen/v1/nico_nico.pb.go is excluded by !**/*.pb.go, !**/gen/**, !rest-api/**/*.pb.go

📒 Files selected for processing (14)

crates/admin-cli/src/operating_system/common.rs
crates/admin-cli/src/operating_system/create/args.rs
crates/admin-cli/src/operating_system/show/cmd.rs
crates/api-core/src/handlers/operating_system.rs
crates/api-core/src/ipxe.rs
crates/api-core/src/tests/instance_os.rs
crates/api-core/tests/integration/operating_system.rs
crates/api-db/migrations/20260717120000_operating_systems_org_nullable.sql
crates/api-db/src/instance.rs
crates/api-db/src/operating_system.rs
crates/api-model/src/operating_system_definition.rs
crates/api-web/src/operating_system.rs
crates/rpc/proto/forge.proto
rest-api/proto/core/src/v1/nico_nico.proto

github-actions · 2026-07-17T21:29:32Z

🔍 Container Scan Summary

Service	Total	Critical	High	Medium	Low	Other
boot-artifacts-aarch64	3	0	0	3	0	0
boot-artifacts-x86_64	3	0	0	3	0	0
forge-admin-cli-x86_64	259	16	29	79	7	128
machine-a-tron	181	7	17	52	6	99
machine-validation-runner	532	39	113	184	15	181
machine_validation	532	39	113	184	15	181
machine_validation-aarch64	532	39	113	184	15	181
nvmetal-carbide	532	39	113	184	15	181
TOTAL	2574	179	498	873	73	951

Per-CVE detail lives in the per-service grype-* artifacts (JSON + SARIF). Severity counts only — no CVE IDs published here.

pbreton · 2026-07-17T23:08:23Z

/retest

pbreton · 2026-07-18T01:07:32Z

/retest

…clear separation of tenant and provider owned OS in core Signed-off-by: Patrice Breton <pbreton@nvidia.com>

Signed-off-by: Patrice Breton <pbreton@nvidia.com>

github-actions · 2026-07-21T01:09:50Z

🌿 Preview your docs: https://nvidia-preview-pull-request-3689.docs.buildwithfern.com/infra-controller

Signed-off-by: Patrice Breton <pbreton@nvidia.com>

hwadekar-nv

This looks good, Thanks @pbreton

pbreton requested a review from a team as a code owner July 17, 2026 19:57

pbreton marked this pull request as draft July 17, 2026 19:57

coderabbitai Bot reviewed Jul 17, 2026

View reviewed changes

Comment thread crates/api-db/src/operating_system.rs Outdated

pbreton marked this pull request as ready for review July 17, 2026 21:46

pbreton added 3 commits July 20, 2026 18:08

chore(core): tenant_organization_id for OS must be optional to allow …

08079c9

…clear separation of tenant and provider owned OS in core Signed-off-by: Patrice Breton <pbreton@nvidia.com>

address coderabbit comments

e6bb842

Signed-off-by: Patrice Breton <pbreton@nvidia.com>

fixes

0e3dfd4

Signed-off-by: Patrice Breton <pbreton@nvidia.com>

pbreton force-pushed the chore/make-tenant_organization_id-optional branch from c7c556f to 0e3dfd4 Compare July 21, 2026 01:08

pbreton mentioned this pull request Jul 21, 2026

feat(rest-api): iPXE template + templated-OS REST endpoints #3569

Draft

3 tasks

kensimon approved these changes Jul 21, 2026

View reviewed changes

pbreton enabled auto-merge (squash) July 21, 2026 17:15

fix conflict

201366e

Signed-off-by: Patrice Breton <pbreton@nvidia.com>

hwadekar-nv approved these changes Jul 21, 2026

View reviewed changes

pbreton merged commit ba9cb2c into NVIDIA:main Jul 21, 2026
165 of 167 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore(core): tenant_organization_id for OS must be optional to allow clean ownership attribution to tenant or provider#3689

chore(core): tenant_organization_id for OS must be optional to allow clean ownership attribution to tenant or provider#3689
pbreton merged 4 commits into
NVIDIA:mainfrom
pbreton:chore/make-tenant_organization_id-optional

pbreton commented Jul 17, 2026 •

edited

Loading

Uh oh!

copy-pr-bot Bot commented Jul 17, 2026

Uh oh!

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading

Uh oh!

pbreton commented Jul 17, 2026

Uh oh!

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 17, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

github-actions Bot commented Jul 17, 2026 •

edited

Loading

Uh oh!

pbreton commented Jul 17, 2026

Uh oh!

pbreton commented Jul 18, 2026

Uh oh!

github-actions Bot commented Jul 21, 2026

Uh oh!

hwadekar-nv left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

pbreton commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of Change

Breaking Changes

Testing

Additional Notes

Uh oh!

copy-pr-bot Bot commented Jul 17, 2026

Uh oh!

coderabbitai Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

pbreton commented Jul 17, 2026

Uh oh!

coderabbitai Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 17, 2026

🔐 TruffleHog Secret Scan

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 Container Scan Summary

Uh oh!

pbreton commented Jul 17, 2026

Uh oh!

pbreton commented Jul 18, 2026

Uh oh!

github-actions Bot commented Jul 21, 2026

Uh oh!

hwadekar-nv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pbreton commented Jul 17, 2026 •

edited

Loading

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading

github-actions Bot commented Jul 17, 2026 •

edited

Loading